Widen dependency versions by pkruithof · Pull Request #64 · Eppo-exp/php-sdk

pkruithof · 2026-07-23T07:33:13Z

🎟️ Fixes issue
📜 Design Doc: link if applicable

Motivation and Context

We're using Eppo with this library and want to upgrade to Symfony 8. Currently the cache dependency is preventing that.

Description

I've widened the requirement for this dependency, and fixed some low-hanging fruit I encountered while doing so:

remove composer.lock as it's not recommended for libraries
expanded the test suite so it tests this library with both the lowest and highest versions of the dependencies

How has this been documented?

How has this been tested?

running make test

pkruithof · 2026-07-23T07:33:45Z

    runs-on: ubuntu-latest
+    strategy:
+      matrix:
+        deps:


Ideally you'd want to add different PHP versions to this as well. I'll leave that out of scope for this PR though.

Not sure why there were no workflows triggered for this PR though, the file syntax seems ok and I don't see any errors in the actions tab.

pkruithof · 2026-07-23T07:36:25Z

 use Psr\Http\Client\ClientExceptionInterface;
 use Psr\Http\Client\ClientInterface;
 use Psr\Http\Message\RequestFactoryInterface;
-use Teapot\StatusCode\RFC\RFC7231;


Having a dependency just for readable http status codes, and enforcing that on all who install this library seems a bit wasteful. But if you insist on keeping this I can revert this, although in that case I would suggest using symfony/http-foundation as it's a far more common dependency.

pkruithof · 2026-07-29T08:45:11Z

@aarsilv you seem to be the most active maintainer here, could you take a look at this please?

aarsilv · 2026-08-10T11:37:57Z

Hey! Apologies for the delay this GitHub notification slipped through the cracks. Change seems reasonable at a high-level, but how come delete whole lockfile (vs. update)? I know composer doesn't use it but we use it for things like security scans.

Copilot

Pull request overview

This pull request updates the library’s dependency constraints to allow newer Symfony Cache versions (targeting Symfony 8 compatibility) and adjusts related tooling/tests to support working without a committed composer.lock.

Changes:

Widen symfony/cache constraint to include ^8.0 and remove the Teapot dependency used only for HTTP status constants.
Remove composer.lock from version control and add it to .gitignore.
Update CI and local test invocation to better support dependency-resolution testing.

Reviewed changes

Copilot reviewed 5 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`composer.json`	Widens `symfony/cache` constraint and removes `shrikeh/teapot` requirement.
`composer.lock`	Deleted to align with library best practices (no committed lockfile).
`.gitignore`	Ignores `composer.lock` going forward.
`.github/workflows/run-tests.yml`	Runs dependency resolution via `composer update` and adds a dependency-mode matrix.
`Makefile`	Switches PHPUnit invocation to `./vendor/bin/phpunit`.
`src/API/APIRequestWrapper.php`	Removes Teapot constants and uses numeric HTTP status ranges.
`tests/API/APIRequestWrapperTest.php`	Removes Teapot constants and inlines HTTP status codes in tests.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

    - name: Install dependencies
-      run: composer install --prefer-dist --no-progress
+      run: composer update --prefer-dist --no-progress ${{ matrix.deps }}


        $redirectHeaders->setHeader(new Header('Location', $redirectLocation));

-        $redirectResponse = new Response(statusCode: RFC7231::MOVED_PERMANENTLY, headers: $redirectHeaders);
+        $redirectResponse = new Response(statusCode: 308, headers: $redirectHeaders);


bertilhatt · 2026-08-10T13:35:17Z

Feedback:

The composer.lock file should be added to the .gitignore, because this is a library and not a project.

Keep composer.lock and prove symfony/cache ^6.4|^7.0|^8.0 works. PR #64 widens that constraint for Symfony 8 support. It also deletes composer.lock. This branch keeps both the widening and the lockfile. #64 https://datadoghq.atlassian.net/browse/FFESUPPORT-934 Deleting the lock was a prerequisite for `composer update` in CI, not for the widening. `composer install` becomes one leg of a matrix instead of the only install mode. The update legs rewrite composer.lock inside the runner, and nothing is committed back. Why the lockfile stays: - GitHub builds the PHP dependency graph from composer.lock. That graph feeds our Dependabot alerts. FFESUPPORT-534, -734 and -887 all came from it. - `composer validate --strict` checks lock sync only when a lock exists. - config.platform.php guards the committed lock. It has no other purpose. Matrix legs and what each resolves: - 8.1, 8.3 locked -> symfony/cache 6.4.40 - 8.1 lowest -> psr/log 2.0.0, psr/cache 2.0.0, google/cloud-storage 1.30.0 - 8.3 highest -> symfony/cache 7.4.16 - 8.4 highest -> symfony/cache 8.1.4 The highest legs unset config.platform.php. The 8.1.0 pin caps the solver at symfony/cache 6.4, so both legs in PR #64 resolved 6.4.x. The ^7.0 and ^8.0 branches went untested. Also: - Add .github/dependabot.yml with grouped updates. Grouping cuts the lockfile churn that makes a lock expensive to keep. - Gate `composer audit` on --no-dev. All 6 current advisories are dev-only. - ramsey/composer-install keys its cache per leg. hashFiles('**/composer.lock') collapsed to a constant once the lock was gone. - Restore 301 in the redirect test. 308 takes a different path in the decorator. composer.lock changes by the Teapot removal only: -105 lines, no version churn. Verified: 98 tests, 664 assertions pass. CI run 31860944824 passed all five legs and produced the resolutions above. codex reports no blocking issues. Out of scope: the 6 dev-only advisories, and test-package.yml running on every branch creation. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

aarsilv · 2026-08-17T21:49:07Z

Hello @pkruithof! I took a stab folding this into #67

Love that you're widening symfony/cache to ^6.4|^7.0|^8.0 and dropping shrikeh/teapot.

Dropping composer.lock is the standard advice for libraries, but GitHub builds our PHP dependency graph from composer.lock and that graph feeds our security alerts. For my linked PR composer install just becomes one leg of the matrix instead of the only install mode. The update legs rewrite the lock inside the runner and nothing is committed back.

Regarding the CI runs, PRs from outside contributors need a maintainer to approve workflows, this is why you couldn't start them.

Anyways, give #67 a look and let us know if it will work for you!

…assert Two findings from CodeRabbit on #67. Set persist-credentials: false on actions/checkout@v5. The action stores the workflow token in local git config by default. Composer and PHPUnit run PR-controlled code, which can read it. No step needs git auth here: make test clones sdk-test-data, which is a separate public repo. Assert an unrecoverable 400 instead of 401. handleHttpError throws InvalidApiKeyException for 401 before it builds HttpRequestException, and that catch branch in assertStatusRecoverable ignores $recoverable. The 401 case therefore asserted nothing, and duplicated testUnauthorizedClient. Proof: assertStatusRecoverable(true, 401) passes, while assertStatusRecoverable(true, 400) fails on the isRecoverable assertion. Both predate this branch. PR #64 touched the lines, so review surfaced them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

pkruithof · 2026-08-24T12:14:03Z

Thanks for checking, looks good to me!

Regarding the composer.lock, it's fine to keep it in: Composer docs do not state that you shouldn't commit it for libraries, just that it's not needed. So in your case I'd keep it in, like you said.

Awaiting the merge/release 🙂

Widen dependency versions

95adc3a

pkruithof commented Jul 23, 2026

View reviewed changes

aarsilv requested a lite review from Copilot August 10, 2026 11:35

Copilot started reviewing on behalf of aarsilv August 10, 2026 11:35 View session

Copilot AI reviewed Aug 10, 2026

View reviewed changes

aarsilv mentioned this pull request Aug 17, 2026

ci(FFESUPPORT-934): test the widened dependency ranges via a matrix #67

Open

Navigation Menu

Uh oh!

Widen dependency versions - #64

Widen dependency versions#64
pkruithof wants to merge 1 commit into
Eppo-exp:mainfrom
pkruithof:widen-dependencies

pkruithof commented Jul 23, 2026

Uh oh!

pkruithof Jul 23, 2026

Uh oh!

pkruithof Jul 23, 2026

Uh oh!

pkruithof Jul 23, 2026

Uh oh!

pkruithof commented Jul 29, 2026

Uh oh!

aarsilv commented Aug 10, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

bertilhatt commented Aug 10, 2026

Uh oh!

aarsilv commented Aug 17, 2026

Uh oh!

pkruithof commented Aug 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

pkruithof commented Jul 23, 2026

Motivation and Context

Description

How has this been documented?

How has this been tested?

Uh oh!

pkruithof Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

pkruithof Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

pkruithof Jul 23, 2026

Choose a reason for hiding this comment

Uh oh!

pkruithof commented Jul 29, 2026

Uh oh!

aarsilv commented Aug 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

bertilhatt commented Aug 10, 2026

Uh oh!

aarsilv commented Aug 17, 2026

Uh oh!

pkruithof commented Aug 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aarsilv commented Aug 10, 2026 •

edited

Loading